request 2 Target for Synerduino boards - #11756
Conversation
Branch Targeting SuggestionYou've targeted the
If This is an automated suggestion to help route contributions to the appropriate branch. |
PR Summary by QodoAdd SYNERDUINO STM32F405 and H743 board targets (HSE 8/25MHz)
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
| #define USE_DSHOT | ||
| #define USE_ESC_SENSOR | ||
| #define USE_SERIAL_4WAY_BLHELI_INTERFACE | ||
|
|
||
| #define TARGET_IO_PORTA 0xffff | ||
| #define TARGET_IO_PORTB 0xffff | ||
| #define TARGET_IO_PORTC 0xffff | ||
| #define TARGET_IO_PORTD (BIT(2)) | ||
|
|
||
| #define MAX_PWM_OUTPUT_PORTS 10 | ||
|
|
||
| #define USE_SERIAL_4WAY_BLHELI_INTERFACE | ||
| #define USE_DSHOT | ||
| #define USE_DSHOT_DMAR | ||
| #define USE_ESC_SENSOR |
There was a problem hiding this comment.
3. Duplicate feature defines 🐞 Bug ⚙ Maintainability
SYNERDUINOSTM_F405/target.h defines USE_DSHOT, USE_ESC_SENSOR, and USE_SERIAL_4WAY_BLHELI_INTERFACE twice, which is redundant and can produce macro redefinition warnings and maintenance confusion. The duplicates don’t add behavior (the macros are identical) but should be collapsed into a single block.
Agent Prompt
### Issue description
At the end of `SYNERDUINOSTM_F405/target.h`, several feature macros are defined twice (`USE_DSHOT`, `USE_ESC_SENSOR`, `USE_SERIAL_4WAY_BLHELI_INTERFACE`). This duplication is unnecessary and makes future changes error-prone.
### Issue Context
The second block also introduces `USE_DSHOT_DMAR`; keep that functionality but avoid repeating the earlier defines.
### Fix Focus Areas
- src/main/target/SYNERDUINOSTM_F405/target.h[239-253]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Test firmware build ready — commit Download firmware for PR #11756 244 targets built. Find your board's
|
|
Please correct the PLL1 and PLL2 configuration. system_stm32h7xx.c:259): pll1Config->m = HSE_VALUE / 1000000 / 2; only works if HSE is a multiple of 2 MHz. With an HSE of 25 MHz, the existing code will incorrectly configure the PLL1 clocks. VCI will be 2.083 MHz instead of 2MHz, VCO will be 1000 MHz instead of 960 MHz and Sysclk will be 500 MHz instead of 480 MHz. PLL2 had an assert added at system_stm32h7xx.c:500-514, when I fixed the silent bug with the Kakute H7 running at 16 MHz. I knew the code I added only worked for 8 MHz and 16 MHz. At both locations the easiest will be to add this structure: |
|
Thanks for these new Synerduino targets! I took a look at this with an AI-based tool I have. It may well be wrong on any of the points below — please treat it as a set of questions to check rather than a verdict, and correct me if I've misread something. I checked out the branch and tried building it to look into a couple of the concerns raised above, and had a few questions. SYNERDUINOH7 — did it actually build for you?
#ifdef USE_SDCARD_SDIO
// PLL2M = HSE_VALUE / 1600000 pins the VCO input to exactly 1.6 MHz for any HSE.
STATIC_ASSERT(HSE_VALUE % 1600000 == 0, HSE_not_a_multiple_of_1600000);25,000,000 % 1,600,000 = 1,000,000, which is nonzero, so I'd expect this to fail to compile for that HSE/SDIO combination. When I tried building yet the CI build worked - weird. Related to @daijoubu's comment above about the PLL1/PLL2 configuration for a 25 MHz HSE — if I'm reading Possible USE_IMU_BMI088 mix-up? (SYNERDUINOSTM_F405 and F405A)Both F405 Possible DMA collision on S5/S6 (SYNERDUINOSTM_F405 and F405A)?
DEF_TIM(TIM1, CH3, PA10, TIM_USE_OUTPUT_AUTO, 0, 0), // S5
DEF_TIM(TIM1, CH2, PA9, TIM_USE_OUTPUT_AUTO, 0, 0), // S6Both use A few smaller things
Really appreciate the work on these targets, especially the dual-gyro "A" variants. Sorry for the wall of questions — happy to be told any/all of this is off base, and glad to help dig into the HSE/PLL2 constants if that ends up being a real issue. |
understood , apparently Ive done some correction on the SPI 1 and SPI 2 pin mixup for the Synerduino H7 boards |
let me know where to add it so I can tests on my 25mhz board |
|
oh and that BMI088 issue is corrected now. |
|
@daijoubu @synersignart I took a look at this with an AI-based tool I have. It may well be wrong on any of the points below — please treat it as a suggestion to check rather than a verdict, and correct me if I've misread something. Following daijoubu's suggested
|
thanks , we'll try it over the weekend . let you know how it goes |
|
Just to clarify, the STATIC_ASSERT is there to catch anyone that added a new target with an HSE that is not 8 or 16 MHz rather than building with a misconfigured clock. There should be a similar one on the PLL1 clock tree as well. AI misread the purpose of the assert. It only allows targets to compile if they will result in the intended clock speeds. I have a task in my backlog to review the flash wait states for Rev V vs Rev Y and will probably add the check for a supported HSE frequency to PLL1 if I need to make any changes there as well at that time. |
|
See attached. I have not tested this. |
@sensei-hacker
here are the request if you can check them out
these are base of the
Weact F405 HSE 8mhz
Weact H743 HSE 25mhz
formerly worked on INAV 9.0.1 however having issues working on INAV 9.1.0